Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feature(rpc): add an rpc server to Zebra #3589

Merged
merged 28 commits into from
Feb 22, 2022
Merged

feature(rpc): add an rpc server to Zebra #3589

merged 28 commits into from
Feb 22, 2022

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Feb 19, 2022 •
edited by teor2345
Loading

Motivation

We want to add an rpc component into zebra so we can start serving rpc commands. The crates we are using are the ones described at #3140

This pull request will close #3140 if the general idea is accepted and the pull request gets merged.

The RPC config also closes #3141.

Solution

A new component was added and a first call getinfo was mocked to test this is working. So this pull request also started the work described in #3141 and #3142 but more work is needed to close those.

This was tested manually (#3589 (comment)), no tests were done as they will be added in the context of #3165

Review

I am mainly interested in feedback for the overall idea, there are currently details to be fixed. Any review from anyone is welcome.

Reviewer Checklist

  • Overall implementation
  • Manual test

Follow Up Work

No further features should be added to this pull request so we can keep the scope small, there is more to do in the other related tickets.

@oxarbitrage
Copy link
Contributor Author

The RPC server of this PR is off by default. A custom config with listen set to true in the rpc section is needed:

...
[rpc]
listen = true
listen_addr = '127.0.0.1:8232'
...

With that in place, when zebrad -c my_config.toml start is executed and everything goes as expected the next lines will be displayed:

...
Feb 19 18:40:53.556  INFO {zebrad="137ae4e" net="Main"}: zebrad::components::rpc: Trying to open RPC endpoint at 127.0.0.1:8232...
...
Feb 19 18:40:53.559  INFO {zebrad="137ae4e" net="Main"}: zebrad::components::rpc: Opened RPC endpoint at 127.0.0.1:8232
...

With zebra running, from a new window one can execute the getinfo RPC command and get a dummy response:

$ curl -X POST -H "Content-Type: application/json" -d '{"jsonrpc": "2.0", "method": "getinfo", "id":123 }' 127.0.0.1:8232
{"jsonrpc":"2.0","result":{"build":"Zebra v1.0.0 ...","subversion":"/Zebra:1.0.0-beta.4/"},"id":123}
$

oxarbitrage
oxarbitrage commented Feb 19, 2022
View reviewed changes
zebra-rpc/Cargo.toml Outdated Show resolved Hide resolved
@oxarbitrage oxarbitrage marked this pull request as draft February 20, 2022 15:32
@codecov
Copy link

codecov bot commented Feb 20, 2022 •
edited
Loading

Codecov Report

Merging #3589 (49f563c) into main (499ae89) will increase coverage by 1.51%.
The diff coverage is 73.45%.

@@            Coverage Diff             @@
##             main    #3589      +/-   ##
==========================================
+ Coverage   78.34%   79.86%   +1.51%     
==========================================
  Files         267      280      +13     
  Lines       31526    32530    +1004     
==========================================
+ Hits        24698    25979    +1281     
+ Misses       6828     6551     -277

teor2345
teor2345 reviewed Feb 20, 2022
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks really good.

I noticed some things that might be bugs:

  • blocking the tokio runtime
  • not checking if the RPC server is still running
  • using the default #[rpc] client derive

I also had some design questions we might want to discuss with the team:

  • does the RPC server code go in zebra-rpc or zebrad?
  • what config should we use to disable the RPC port?

zebra-rpc/Cargo.toml Outdated Show resolved Hide resolved
zebrad/src/commands/start.rs Show resolved Hide resolved
zebrad/src/components/rpc/config.rs Outdated Show resolved Hide resolved
zebrad/src/components/rpc.rs Outdated Show resolved Hide resolved
zebrad/src/components/rpc.rs Outdated Show resolved Hide resolved
zebrad/src/components/rpc.rs Outdated Show resolved Hide resolved
zebrad/src/commands/start.rs Outdated Show resolved Hide resolved
zebra-rpc/src/rpc.rs Outdated Show resolved Hide resolved
zebrad/src/components/rpc.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 added A-dependencies Area: Dependency file updates A-rpc Area: Remote Procedure Call interfaces C-enhancement Category: This is an improvement P-Medium ⚡ labels Feb 20, 2022
@teor2345 teor2345 mentioned this pull request Feb 20, 2022
Merged
1 task
@teor2345
Copy link
Contributor

teor2345 commented Feb 20, 2022 •
edited
Loading

With zebra running, from a new window one can execute the getinfo RPC command and get a dummy response:

$ curl -X POST -H "Content-Type: application/json" -d '{"jsonrpc": "2.0", "method": "getinfo", "id":123 }' 127.0.0.1:8232
{"jsonrpc":"2.0","result":{"build":"Zebra v1.0.0 ...","subversion":"/Zebra:1.0.0-beta.4/"},"id":123}
$

This tests for JSON RPC 2.0, but zcashd and lightwalletd use JSON RPC 1.0, which has a different format:

$ curl -H "Content-Type: application/json" -d '{"method": "getinfo", "params":[], "id":123 }' 127.0.0.1:8232
{"result":{"build":"Zebra v1.0.0 ...","subversion":"/Zebra:1.0.0-beta.4/"},"id":123}

@teor2345 teor2345 mentioned this pull request Feb 21, 2022
Merged
3 tasks
@teor2345
Copy link
Contributor

teor2345 commented Feb 21, 2022 •
edited
Loading

I did some testing with lightwalletd today.

It sends an incorrect "jsonrpc: 1.0" field in requests, so I created this fix for Zebra:
oxarbitrage#173

With this config file:

rpcbind=127.0.0.1
rpcport=38232

Before the fix, I see:

$ ./lightwalletd --no-tls-very-insecure --zcash-conf-path ~/.config/zebra/zebrad-testnet-lightwalletd-zcash.conf --log-file /dev/stdout --data-dir $(mktemp -d)
{"app":"lightwalletd","buildDate":"2022-02-21","buildUser":"dev","gitCommit":"631bb16404e3d8b045e74a7c5489db626790b2f6","level":"info","msg":"Starting gRPC server version v0.4.9-3-g631bb16 on 127.0.0.1:9067","time":"2022-02-21T11:33:34+10:00"}
{"app":"lightwalletd","level":"warning","msg":"Starting insecure no-TLS (plaintext) server","time":"2022-02-21T11:33:34+10:00"}
Starting insecure server
{"app":"lightwalletd","error":"-32600: Invalid request","level":"warning","msg":"error with getblockchaininfo rpc, retrying...","retry":1,"time":"2022-02-21T11:33:34+10:00"}
(then repeated retries)

This is a bug in Zebra, we should accept the request.

After the fix, I see:

$ ./lightwalletd --no-tls-very-insecure --zcash-conf-path ~/.config/zebra/zebrad-testnet-lightwalletd-zcash.conf --log-file /dev/stdout --data-dir $(mktemp -d)
{"app":"lightwalletd","buildDate":"2022-02-21","buildUser":"dev","gitCommit":"631bb16404e3d8b045e74a7c5489db626790b2f6","level":"info","msg":"Starting gRPC server version v0.4.9-3-g631bb16 on 127.0.0.1:9067","time":"2022-02-21T12:46:26+10:00"}
{"app":"lightwalletd","level":"warning","msg":"Starting insecure no-TLS (plaintext) server","time":"2022-02-21T12:46:26+10:00"}
Starting insecure server
{"app":"lightwalletd","level":"info","msg":"Got sapling height 0 block height 0 chain main branchID ","time":"2022-02-21T12:46:26+10:00"}
{"app":"lightwalletd","level":"info","msg":"Found 0 blocks in cache","time":"2022-02-21T12:46:26+10:00"}
{"app":"lightwalletd","error":"-32601: Method not found","level":"fatal","msg":"error zcashd getbestblockhash rpc","time":"2022-02-21T12:46:26+10:00"}
Lightwalletd died with a Fatal error. Check logfile for details.

This failure is the correct behaviour, because the getbestblockhash RPC isn't implemented yet.

teor2345
teor2345 reviewed Feb 22, 2022
View reviewed changes
zebra-rpc/src/config.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 self-assigned this Feb 22, 2022
@teor2345 teor2345 mentioned this pull request Feb 22, 2022
Closed
@teor2345
Copy link
Contributor

I did some testing with lightwalletd today.

I repeated these tests with commit 49f563c in this PR.

I got the same response from lightwalletd. So I am happy to merge this PR without integration tests.

This was referenced Feb 22, 2022
Closed
Closed
teor2345
teor2345 approved these changes Feb 22, 2022
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a bunch of cleanups, but the original code was fine and working.

I tested a ZcashFoundation repository PR and all the tests passed:

So it looks like the other test failures were due to repository secrets bugs, we've got tickets for those.

@oxarbitrage oxarbitrage marked this pull request as ready for review February 22, 2022 11:25
@oxarbitrage
Copy link
Contributor Author

Nice work, thanks @teor2345

@oxarbitrage oxarbitrage merged commit 8e36686 into ZcashFoundation:main Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 approved these changes

Assignees

@teor2345 teor2345

@oxarbitrage oxarbitrage

Labels
A-dependencies Area: Dependency file updates A-rpc Area: Remote Procedure Call interfaces C-enhancement Category: This is an improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make the JSON-RPC listener port configurable Add a JSON-RPC listener port and listener component
2 participants
@oxarbitrage @teor2345